Adds comet-ml plugin #2550
Adds comet-ml plugin #2550
Conversation
Signed-off-by: Thomas J. Fan <[email protected]>
Signed-off-by: Thomas J. Fan <[email protected]>
Signed-off-by: Thomas J. Fan <[email protected]>
Signed-off-by: Thomas J. Fan <[email protected]>
Signed-off-by: Thomas J. Fan <[email protected]>
thomasjpfan
requested review from
wild-endeavor,
kumare3,
eapolinario,
pingsutw,
cosmicBboy,
samhita-alla and
Future-Outlier
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2550 +/- ##
===========================================
+ Coverage 52.89% 90.41% +37.52%
===========================================
Files 297 60 -237
Lines 23957 3058 -20899
Branches 3655 0 -3655
===========================================
- Hits 12671 2765 -9906
+ Misses 11178 293 -10885
+ Partials 108 0 -108 ☔ View full report in Codecov by Sentry. |
| # For local execution, always use the experiment_key. If `self.experiment_key` is `None`, comet_ml | ||
| # will generate it's own key | ||
| if self.experiment_key is not None: | ||
| init_kwargs["experiment_key"] = self.experiment_key |
There was a problem hiding this comment.
is api key not required for local executions?
There was a problem hiding this comment.
It's required, I use export COMET_API_KEY="xxxx" to test it in local execution.
| raise ValueError("project_name must be set") | ||
| if workspace is None: | ||
| raise ValueError("workspace must be set") | ||
| if secret is None: |
There was a problem hiding this comment.
shouldn't secret be made optional as it isn't necessary when executing locally?
There was a problem hiding this comment.
The secret field is ignored during location execution, but required during remote. One still needs export COMET_API_KEY= for local execution.
If this was optional, we'll catch the "no secret" during execution and not at registration time. I do not want to wait until then, because these experiments can use GPUs and waiting & spinning up a GPU just to see this error is not efficient and a worst DevX.
By making secret required, I am optimizing for "failing quickly" for remote execution.
Signed-off-by: Thomas J. Fan <[email protected]>
…ml_init Signed-off-by: Thomas J. Fan <[email protected]>
plugins/flytekit-comet-ml/README.md
Outdated
| dynamic-log-links: | ||
| - comet-ml-execution-id: | ||
| displayName: Comet | ||
| templateUris: {{ .taskConfig.host }}/{{ .taskConfig.workspace }}/{{ .taskConfig.project_name }}/{{ .executionName }}{{ .nodeId }}{{ .taskRetryAttempt }}{{ .taskConfig.link_suffix }} |
There was a problem hiding this comment.
we should use double quote here, right?
plugins/flytekit-comet-ml/README.md
Outdated
| templateUris: {{ .taskConfig.host }}/{{ .taskConfig.workspace }}/{{ .taskConfig.project_name }}/{{ .executionName }}{{ .nodeId }}{{ .taskRetryAttempt }}{{ .taskConfig.link_suffix }} | ||
| - comet-ml-custom-id: | ||
| displayName: Comet | ||
| templateUris: {{ .taskConfig.host }}/{{ .taskConfig.workspace }}/{{ .taskConfig.project_name }}/{{ .taskConfig.experiment_key }} |
| project_name: Optional[str] = None, | ||
| workspace: Optional[str] = None, | ||
| experiment_key: Optional[str] = None, | ||
| secret: Optional[Union[Secret, Callable]] = None, |
There was a problem hiding this comment.
Can project_name, workspace, and secret be None Optional here in the type hints?
It will be better for the type hints
There was a problem hiding this comment.
They are already Optional here. Are you suggesting to not make them Optional?
There was a problem hiding this comment.
I got it to work with 417d03b It forces project_name and friends to be required.
|
@thomasjpfan https://github.com/flyteorg/flytekit/blob/master/.github/workflows/pythonbuild.yml#L319 |
Signed-off-by: Thomas J. Fan <[email protected]>
thomasjpfan
force-pushed
the
thomasjpfan/cometml_init
branch
from
0eb34fa to
cc2e9ad
Compare
* Adds comet-ml plugin Signed-off-by: Thomas J. Fan <[email protected]> * For local execution, do not set experiment_key if it is none Signed-off-by: Thomas J. Fan <[email protected]> * Use correct comet-ml links Signed-off-by: Thomas J. Fan <[email protected]> * Allow host to be adjustable Signed-off-by: Thomas J. Fan <[email protected]> * Adds comet-ml plugin Signed-off-by: Thomas J. Fan <[email protected]> * Use new comet-ml login name Signed-off-by: Thomas J. Fan <[email protected]> * Require the project_name workspace and secrets Signed-off-by: Thomas J. Fan <[email protected]> --------- Signed-off-by: Thomas J. Fan <[email protected]> Signed-off-by: mao3267 <[email protected]>
Why are the changes needed?
This PR adds a comet-ml plugin to flytekit.
What changes were proposed in this pull request?
This PR adds a
comet_ml_inittask decorator that adds a link to the Comet's platform.How was this patch tested?
Unit tests were added to this PR.
Screenshots
Related PRs
Related to #2405
Docs link
I also added an example in flytesnacks: flyteorg/flytesnacks#1702